New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: remove esModuleInterop from tsconfig (#110) #111
Conversation
Codecov Report
@@ Coverage Diff @@
## master #111 +/- ##
=======================================
Coverage 97.57% 97.57%
=======================================
Files 6 6
Lines 700 702 +2
Branches 299 299
=======================================
+ Hits 683 685 +2
Misses 15 15
Partials 2 2
Continue to review full report at Codecov.
|
import addAbsolutePathKeyword from './keywords/absolutePath'; | ||
|
||
import ValidationError from './ValidationError'; | ||
|
||
const Ajv = require('ajv'); | ||
const ajvKeywords = require('ajv-keywords'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way TypeScript implements module imports is by following the ECMAScript module standard. In contrast, Node.js uses CommonJS, with exports
and module.exports
. Since TypeScript is supposed to work for both Node and Browser (any JS runtime), it should by default follow the language standard of ESModules.
For cases where the author wants to use the import syntax where the library being imported exports via CommonJS, TypeScript offers the esModuleInterop
flag, to tell the compiler to treat module.exports =
as an ES modules export default
.
This gets to the root of the issue. The ajv author is not adhering to the JS language's now-official ES Modules standards. See here and here, so in order to solve the issue I opened (#110), we have two possible approaches:
- Change the
schema-utils
package so it no longer usesesModuleInterop
in its tsconfig, so we can ensure that any typescript definitions generated vianpm run build:types
will result in definitions which don't require direct or transitive consumers to also enable thisesModuleInterop
flag - Convince the ajv author to make a breaking change so that modules are exported using the ES Modules standard.
So in order to solve the issue without any breaking changes to any package, I've made the change you commented on: replacing the ambiguous import
with require
. Require has and will always be a CommonJS thing sharing no overlap with the ES Modules spec, so TypeScript can immediately interpret require
as a CommonJS import without the need for any esModuleInterop
flag.
If I didn't make this change to require
, while still trying to remove the esModuleInterop
flag, then this repo's npm run build:types
would have failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexleung let's add comments about it to code to avoid misleading for other developers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment above those two require lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
This PR contains a:
Motivation / Use-Case
#110
Breaking Changes
none
Additional Info
allows direct and transitive consumers of this dependency to no longer be required to add
esModuleInterop
to their tsconfig.json